Try casting options, etc to strings #7133
Conversation
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
peterwilsoncc
force-pushed
the
try/options-types-caching-etc
branch
2 times, most recently
from
6238c9f to
1ef018d
Compare
There was a problem hiding this comment.
@peterwilsoncc I am concerned about this change, mostly because it changes the return type of the option retrieval functions, which I think is notable backward compatibility breakage. The few get_option() instances you have to update in this PR alone indicate that.
This is an extremely difficult problem to solve, but I also feel like this PR does more than https://core.trac.wordpress.org/ticket/32848 would need? Why not focus specifically on the null / isset() problem related to wp_load_alloptions()?
| @@ -900,7 +902,7 @@ function update_option( $option, $value, $autoload = null ) { | |||
| * | |||
| * See https://core.trac.wordpress.org/ticket/38903 | |||
| */ | |||
| if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) { | |||
| if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) { | |||
There was a problem hiding this comment.
I am concerned about this, for the same reason that https://core.trac.wordpress.org/ticket/22192 didn't work out the way we were hoping. There is definitely room for BC breakage here.
$old_value is not necessarily a string or non-scalar value. The return value of get_option() can be filtered, and it's not mandated anywhere to return a string. And even without custom filters, this can commonly be another type, particularly when the default is returned that is specified via register_setting() and injected via core's own filter usage.
There was a problem hiding this comment.
Also, why are we using $serialized_value in the first clause? $old_value may be non-scalar in which case this is never true.
Could this maybe work instead?
| if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) { | |
| if ( | |
| $value === $old_value || | |
| ( is_scalar( $value ) && is_scalar( $old_value ) && sprintf( '%s', $value ) === sprintf( '%s', $old_value ) ) || | |
| maybe_serialize( $old_value ) === $serialized_value ) { |
This seems a bit safer to me as it maintains both of the original clauses and only considers the problem case as a new clause for scalar values.
| * option is stored in the database. Rather than type casting, sprintf is used to | ||
| * match the process used by wpdb::prepare(). | ||
| */ | ||
| $serialized_value = sprintf( '%s', $serialized_value ); |
There was a problem hiding this comment.
As far as I can tell, the $serialized_value variable isn't used anywhere between lines 905 and 923. Could we move this right below the clause in 905 to better colocated the code that adjusts the option value to store?
| @@ -2368,7 +2398,7 @@ function update_network_option( $network_id, $option, $value ) { | |||
| * | |||
| * See https://core.trac.wordpress.org/ticket/44956 | |||
| */ | |||
| if ( $value === $old_value || maybe_serialize( $value ) === maybe_serialize( $old_value ) ) { | |||
| if ( sprintf( '%s', $serialized_value ) === $old_value || maybe_serialize( $old_value ) === $serialized_value ) { | |||
There was a problem hiding this comment.
See my comment above, this would apply here as well.
peterwilsoncc
force-pushed
the
try/options-types-caching-etc
branch
from
ef64da1 to
63218fd
Compare
peterwilsoncc
force-pushed
the
try/options-types-caching-etc
branch
2 times, most recently
from
755f59f to
ef2859d
Compare
Cache key. CS I will never remember.
peterwilsoncc
force-pushed
the
try/options-types-caching-etc
branch
from
ef2859d to
40c7bde
Compare
I've pushed some tests to demonstrate the issue I'm trying to resolve. The return type for the options getters is currently inconsistent depending on whether the cache is warm or cold. See the action run after pushing the test changes only. Anything that uses The tests are showing that that's still the case in some places so I will look at those and come back to the rest of your feedback and suggestions. |
This casts the cached value of options to a string in
update_option(),update_network_option(),add_option()andadd_network_option().Currently the cached value of an option can be in either of the original type or a string, depending on whether the cache was primed after a database query in
get_(network)_option()(always a string) or primed while updating or adding an option (type as passed to the function).The effect of this is that the result of
get_(network)_option()can be inconsistent depending on whether the cache was primed or required a database query.This change ensures that the options always use the same type (a string) regardless of how the cache is primed.
Risks
Yes.
On hosting providers that prevent the options cache/all options cache from being flushed the type would consistently be as set by the code while adding or updating the value. Following this change, that will no longer be the case.
Trac ticket: https://core.trac.wordpress.org/ticket/32848
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.